Draft: User config #1704

cathales · 2023-12-13T09:02:47Z

⚠️ This PR is not ready for merge yet. It builds CVA6 with embedded configuration on Verilator and runs CoreMark but there are still formatting + history cleanup + rebase + fixes to do for VCS and other configurations.

This PR is a step towards CVA6 parametrization.

It distinguishes two types: "user configuration" VS "extended configuration", which is interesting because:
- It is not needed anymore to insert dummy values into a structure if the configuration file.
- It appears in the type signature that configuration has been extended or not: when the extended configuration is needed, the appropriate type is used to force that the transmitted parameter is already extended, avoiding the accidental use of dummy values.
It creates a function to gather the extension logic.
It moves user-config-dependent information from packages to this configuration structure as it is not possible for packages to use values from a configuration structure.

Not everything has been moved yet, but as moving XLEN (and its dependents PLEN, VLEN and many more types and values) implied a lot of changes, it is better to commit them as soon as it is working to avoid an even bigger PR (and the associated conflicts).

github-actions

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

verible-verilog-format

github-actions · 2023-12-13T09:03:37Z

core/acc_dispatcher.sv

@@ -18,8 +18,40 @@ module acc_dispatcher
  import riscv::*;
 #(
    parameter config_pkg::cva6_cfg_t CVA6Cfg    = config_pkg::cva6_cfg_empty,


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

parameter config_pkg::cva6_cfg_t CVA6Cfg = config_pkg::cva6_cfg_empty,

parameter config_pkg::cva6_cfg_t CVA6Cfg = config_pkg::cva6_cfg_empty,

github-actions · 2023-12-13T09:03:38Z

core/acc_dispatcher.sv

-    parameter type                   acc_req_t  = acc_pkg::accelerator_req_t,
-    parameter type                   acc_resp_t = acc_pkg::accelerator_resp_t,
+    parameter type exception_t = logic,
+    parameter type                   fu_data_t  = logic,


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

parameter type fu_data_t = logic,

parameter type fu_data_t = logic,

github-actions · 2023-12-13T09:03:38Z

core/acc_dispatcher.sv

+      logic                                 req_valid;
+      logic                                 resp_ready;
+      riscv::instruction_t                  insn;
+      logic [CVA6Cfg.XLEN-1:0]                         rs1;
+      logic [CVA6Cfg.XLEN-1:0]                         rs2;
+      fpnew_pkg::roundmode_e                frm;


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

logic req_valid;

logic resp_ready;

riscv::instruction_t insn;

logic [CVA6Cfg.XLEN-1:0] rs1;

logic [CVA6Cfg.XLEN-1:0] rs2;

fpnew_pkg::roundmode_e frm;

logic req_valid;

logic resp_ready;

riscv::instruction_t insn;

logic [CVA6Cfg.XLEN-1:0] rs1;

logic [CVA6Cfg.XLEN-1:0] rs2;

fpnew_pkg::roundmode_e frm;

github-actions · 2023-12-13T09:03:38Z

core/acc_dispatcher.sv

+      logic [CVA6Cfg.XLEN-1:0]                         rs2;
+      fpnew_pkg::roundmode_e                frm;
+      logic [CVA6Cfg.TRANS_ID_BITS-1:0] trans_id;
+      logic                                 store_pending;


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

logic store_pending;

logic store_pending;

github-actions · 2023-12-13T09:03:38Z

core/acc_dispatcher.sv

+      logic                                 acc_cons_en;
+      logic                                 inval_ready;


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

logic acc_cons_en;

logic inval_ready;

logic acc_cons_en;

logic inval_ready;

github-actions · 2023-12-13T09:03:41Z

core/alu.sv

-          CLZW, CTZW: result_o = (lz_tz_wempty) ? 32 : {{riscv::XLEN - 5{1'b0}}, lz_tz_wcount}; // change
-          ROLW: result_o = {{riscv::XLEN - 32{rolw[31]}}, rolw};
-          RORW, RORIW: result_o = {{riscv::XLEN - 32{rorw[31]}}, rorw};
+          CLZW, CTZW: result_o = (lz_tz_wempty) ? 32 : {{CVA6Cfg.XLEN - 5{1'b0}}, lz_tz_wcount}; // change


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

CLZW, CTZW: result_o = (lz_tz_wempty) ? 32 : {{CVA6Cfg.XLEN - 5{1'b0}}, lz_tz_wcount}; // change

CLZW, CTZW:

result_o = (lz_tz_wempty) ? 32 : {{CVA6Cfg.XLEN - 5{1'b0}}, lz_tz_wcount}; // change

github-actions · 2023-12-13T09:03:41Z

core/alu.sv

+        result_o = (lz_tz_empty) ? ({{CVA6Cfg.XLEN - $clog2(CVA6Cfg.XLEN) {1'b0}}, lz_tz_count} + 1) :
+            {{CVA6Cfg.XLEN - $clog2(CVA6Cfg.XLEN) {1'b0}}, lz_tz_count};


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

result_o = (lz_tz_empty) ? ({{CVA6Cfg.XLEN - $clog2(CVA6Cfg.XLEN) {1'b0}}, lz_tz_count} + 1) :

{{CVA6Cfg.XLEN - $clog2(CVA6Cfg.XLEN) {1'b0}}, lz_tz_count};

result_o = (lz_tz_empty) ? ({{CVA6Cfg.XLEN - $clog2(CVA6Cfg.XLEN) {1'b0}}, lz_tz_count} + 1)

: {{CVA6Cfg.XLEN - $clog2(CVA6Cfg.XLEN) {1'b0}}, lz_tz_count};

github-actions · 2023-12-13T09:03:41Z

core/amo_buffer.sv

@@ -39,8 +39,8 @@ module amo_buffer #(

  typedef struct packed {
    ariane_pkg::amo_t       op;


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

ariane_pkg::amo_t op;

ariane_pkg::amo_t op;

github-actions · 2023-12-13T09:03:41Z

core/amo_buffer.sv

+    logic [CVA6Cfg.XLEN-1:0]           data;
    logic [1:0]             size;


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

logic [CVA6Cfg.XLEN-1:0] data;

logic [1:0] size;

logic [CVA6Cfg.XLEN-1:0] data;

logic [1:0] size;

github-actions · 2023-12-13T09:03:41Z

core/amo_buffer.sv

      .DEPTH(1),
-      .dtype(amo_op_t)
+      .dtype(amo_op_t),


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

.DEPTH(1),

.dtype(amo_op_t)

.dtype(amo_op_t),

.DEPTH (1),

.dtype (amo_op_t),

github-actions · 2023-12-13T09:06:47Z

❌ failed run, report available here.

cathales · 2023-12-13T11:05:38Z

Note after discussion with @AngelaGonzalezMarino:

I will change how I define types before spreading them. I have not embraced a specific style yet and I am not consistent. I’m sometimes defining a type with parameter even if it is not supposed to be taken as a generic, which is confusing. Three options to consider:

At least use localparam instead of parameter
Move the localparam into the module body (instead of in the #() generic parameters)
Use a typedef instead of a localparam type, but then I’m not sure if transmitting it will be correctly supported.

I would opt for option 2: localparam in the module body.

Thanks a lot to @AngelaGonzalezMarino for the review!

This way the CV-X-IF agent (in CoreV Verif repository) will be able to get parameters from CVA6Cfg once parametrized. Once the agent is parametrized, it will be possible to remove the CV-X-IF parameters from the package and continue with CVA6 parametrization. As discussed during 2023-12-14 verification meeting.

github-actions

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

verible-verilog-format

github-actions · 2023-12-14T13:40:20Z

core/cache_subsystem/cache_ctrl.sv


  always_comb begin : way_select
    cl_i = '0;
-    for (int unsigned i = 0; i < DCACHE_SET_ASSOC; i++) if (hit_way_i[i]) cl_i = data_i[i].data;
+    for (int unsigned i = 0; i < CVA6Cfg.DCACHE_SET_ASSOC; i++) if (hit_way_i[i]) cl_i = data_i[i].data;


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

for (int unsigned i = 0; i < CVA6Cfg.DCACHE_SET_ASSOC; i++) if (hit_way_i[i]) cl_i = data_i[i].data;

for (int unsigned i = 0; i < CVA6Cfg.DCACHE_SET_ASSOC; i++)

if (hit_way_i[i]) cl_i = data_i[i].data;

github-actions · 2023-12-14T13:40:20Z

core/cache_subsystem/cva6_hpdcache_subsystem.sv

+    parameter type icache_arsp_t = logic,
+    parameter type icache_dreq_t = logic,
+    parameter type icache_drsp_t = logic,
+    parameter type icache_req_t  = logic,


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

parameter type icache_req_t = logic,

parameter type icache_req_t = logic,

github-actions · 2023-12-14T13:40:20Z

core/cache_subsystem/cva6_hpdcache_subsystem.sv

    input  cmo_req_t                                 dcache_cmo_req_i,    // from CMO FU
    output cmo_rsp_t                                 dcache_cmo_resp_o,   // to CMO FU


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

input cmo_req_t dcache_cmo_req_i, // from CMO FU

output cmo_rsp_t dcache_cmo_resp_o, // to CMO FU

input cmo_req_t dcache_cmo_req_i, // from CMO FU

output cmo_rsp_t dcache_cmo_resp_o, // to CMO FU

github-actions · 2023-12-14T13:40:20Z

core/cache_subsystem/cva6_hpdcache_subsystem.sv

+    input  dcache_req_i_t [NumPorts-1:0] dcache_req_ports_i,  // from LSU
+    output dcache_req_o_t [NumPorts-1:0] dcache_req_ports_o,  // to LSU


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

input dcache_req_i_t [NumPorts-1:0] dcache_req_ports_i, // from LSU

output dcache_req_o_t [NumPorts-1:0] dcache_req_ports_o, // to LSU

input dcache_req_i_t [NumPorts-1:0] dcache_req_ports_i, // from LSU

output dcache_req_o_t [NumPorts-1:0] dcache_req_ports_o, // to LSU

github-actions · 2023-12-14T13:40:20Z

core/cache_subsystem/cva6_hpdcache_subsystem.sv

    output logic                                     wbuffer_empty_o,
    output logic                                     wbuffer_not_ni_o,


[verible-verilog-format] _{reported by reviewdog 🐶}

Suggested change

output logic wbuffer_empty_o,

output logic wbuffer_not_ni_o,

output logic wbuffer_empty_o,

output logic wbuffer_not_ni_o,

github-actions · 2023-12-14T13:40:23Z